-
Notifications
You must be signed in to change notification settings - Fork 422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix iOS image loading process outputting alpha-premultiplied colours #6454
Conversation
|
||
// unapply alpha pre-multiplication resulted by CGBitmapContext. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a quick read it doesn't look like there's a CGBitmapContext
anymore?
Also just a general question: did you use some other code as reference for this? It would be good to have some links to other similar implementation to reassure me that this isn't completely back to front (because I feel like it might be, and that the correct method is to adjust things so we can set the src/dest blend to support this at a metal level).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a quick read it doesn't look like there's a
CGBitmapContext
anymore?
Yeah this comment is stale, should mention CIContext
instead.
Also just a general question: did you use some other code as reference for this? It would be good to have some links to other similar implementation to reassure me that this isn't completely back to front (because I feel like it might be, and that the correct method is to adjust things so we can set the src/dest blend to support this at a metal level).
This is not referenced from anywhere else, it was written as such after various attempts of internet searching to achieve the desired behaviour, specifically after being aware of CIContext
in a random stack overflow thread (I should also mention that CGBitmapContext
supports floating-point components but the numbers are completely imprecise, seemingly converted from premultiplied byte values).
As to whether this is back to front or not, it is back to front in terms of iOS/Metal most likely. As I've read more about this before submitting this PR it does seem like the correct approach is to configure Metal to blend premultiplied colours more correctly, but I've gone with this PR first as I have a feeling it will not be a simple one at all. The masking/texture shaders will probably have to be thoroughly re-checked to handle premultiplied colours correctly, alongside figuring out what it takes to make Metal blend premultiplied colours.
I'll investigate that today and see where it leads me.
After benchmarking this, it turns out it's completely slow. Using |
Marking as draft pending performance profiling and figuring out why there are still noticeable artifacts in intro even with this PR...;
By the nature of iOS, no image can be provided as raw pixel data without the colour components already being alpha-premultiplied, so we have to unapply that to have things working.
Since the processing logic was directly converting the image to raw pixel data in
RGBA8
format, premultiplication becomes a very lossy process as the alpha component approaches zero. In other words, a pixel of(79, 79, 79, 4)
when premultiplied becomes ~(1, 1, 1, 4)
, unapplying on the result yields(64, 64, 64, 4)
, which is off from the original by 15 degrees.Therefore, I've rewritten the logic to render in floating-point format, giving enough precision to handle the case above. I'm not 100% sure about the performance implications of this but I would like to hope it's far less than loading the image through ImageSharp.